Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

docs(bigquery): fix the broken docs #139

Merged
merged 2 commits into from
Jun 19, 2020

Conversation

HemangChothani
Copy link
Contributor

Fixes #138

-> I know conf.py file generated by synthtool ,this is temporary patch for bigquery.

-> new release of sphinx 3.1.1 Consider duplicate docstring of Attributes: docstring and property docstring and raised an error of duplicate description so change the docstring from Attributes: to Args for more specific info refer this issue : sphinx-doc/sphinx#7817

@HemangChothani HemangChothani requested a review from plamut June 18, 2020 11:59
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Jun 18, 2020
@HemangChothani
Copy link
Contributor Author

Opened a PR in synthtool to update conf.py file template googleapis/synthtool#629

Copy link
Contributor

@plamut plamut left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know conf.py file generated by synthtool ,this is temporary patch for bigquery.

How about making the change in synth.py and change the key line in docs/conf.py file automatically?

The fix itself work, though, thanks for that!

@HemangChothani
Copy link
Contributor Author

@plamut The PR of synthtool googleapis/synthtool#629 has been merged so i will remove the doc/conf.py change from here, but i don't have an idea about change in synth.py file.

@plamut
Copy link
Contributor

plamut commented Jun 19, 2020

@HemangChothani I presume we still need to add the "inherited-members": True option to the docs config, because the synth template contains just the "members": True option? Re-generating the files with synthtool erases that latter part, thus we need to add it manually (through a replacement configured in synth.py)

Mentioning this, because running the synthtool produces the following change (among others):

diff --git docs/conf.py docs/conf.py
index 30dcac5..91b39ac 100644
--- docs/conf.py
+++ docs/conf.py
@@ -43,7 +43,7 @@ extensions = [
 
 # autodoc/autosummary flags
 autoclass_content = "both"
-autodoc_default_options = {"members": True, "inherited-members": True}
+autodoc_default_options = {"members": True}
 autosummary_generate = True

Or do you think we could actually make that change in the docs/conf.py template itself? (don't know if that's suitable for all Python client libs, though)

autodoc_default_options = {"members": True, "inherited-members": True}

@HemangChothani
Copy link
Contributor Author

@plamut
In synth.py file can we do that? I am not sure right now as i am struggling with synth config to local machine.

s.replace(
"docs/conf.py",
{"members": True},
{"members": True, "inherited-members": True}
)

I am not sure that it will work for all clients or not , i will test it.

@plamut
Copy link
Contributor

plamut commented Jun 19, 2020

@HemangChothani Yes, each library can add custom modifications to the generated files in its synth.py.

The snippet above is the right direction, just mind that the second argument is treated as a regular expression, thus regex special characters need to be escaped.

If you have trouble setting up synth locally, I can post a patch here myself to save time, as I already happen to have everything working locally.

@MaxxleLLC MaxxleLLC force-pushed the bigquery_issue_138 branch from e999c20 to 6d95afb Compare June 19, 2020 11:02
@HemangChothani
Copy link
Contributor Author

@plamut Thank you for the guidance and support, i am able to run locally an test it and push the code, could you please verify it?

Copy link
Contributor

@plamut plamut left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regex pattern needs to be updated... if running the synthtool locally, doesn't it emit a warning that a replacement might no longer be needed?

synth.py Outdated Show resolved Hide resolved
noxfile.py Outdated Show resolved Hide resolved
@MaxxleLLC MaxxleLLC force-pushed the bigquery_issue_138 branch from 6d95afb to 19adf86 Compare June 19, 2020 11:19
Copy link
Contributor

@plamut plamut left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good now. The classes in the generated docs again have fully documented members, and re-generating the code preserves the relevant changes in docs/conf.py.

Thanks for adding the suggested improvements quickly!

@HemangChothani HemangChothani merged commit 3235255 into googleapis:master Jun 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[docs][1.25] Documentation on https://googleapis.dev/python/bigquery/1.25.0/ is broken
3 participants